Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Restructure the Solution for 'Army of Functions' task and Fix Typos #2081

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
iliakan merged 2 commits into javascript-tutorial:master from MuhammedZakir:master
Sep 10, 2020

Conversation

@MuhammedZakir
Copy link
Contributor

@MuhammedZakir MuhammedZakir commented Aug 25, 2020 •
edited
Loading

Fix #2068 - Army of Functions
Fix #2070 - Typo
Fix #2056 - Grammatical Error
Fix #2074 - Remove semi-colon after function declaration

Copy link

CLAassistant commented Aug 25, 2020 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

Copy link
Member

iliakan commented Aug 27, 2020

Anything wrong with the SVG?

Copy link
Contributor Author

MuhammedZakir commented Sep 1, 2020 •
edited
Loading

Encountered some problem on my end. Everything fixed now!

Anything wrong with the SVG?

I fixed it by replacing the SVG with a new one!

@MuhammedZakir MuhammedZakir marked this pull request as draft September 1, 2020 10:23
@MuhammedZakir MuhammedZakir marked this pull request as ready for review September 1, 2020 11:51
Copy link
Member

@iliakan iliakan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Please see the comments.
  2. Split PR please. One PR covers a lot. This way I can accept it partially.


```js run demo
function makeArmy() {
As you can see above, on each iteration of a `while {...} ` block, a new lexical environment is created. This implies that as long as we store the value of `i` in a variable in the `while {...}` block, created Lexical Environment will have that variable with value of `i`.
Copy link
Member

@iliakan iliakan Sep 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second phrase is hard to understand.

Copy link
Contributor Author

@MuhammedZakir MuhammedZakir Sep 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

created Lexical Environment will have that variable with value of i.

^This?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies that as long as we store the value of i in a variable in the while {...} block, created Lexical Environment will have that variable with value of i.

Copy link
Contributor Author

@MuhammedZakir MuhammedZakir Sep 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the entire sentence not making any sense or is it the way it is phrased? Do you have any idea how to simplify it? I couldn't think of anything atm. :-(

P.S. I think people can grasp the meaning if it is read along with the snippet below.

Copy link
Member

iliakan commented Sep 3, 2020

Please make different PRs. This one is quite big for review, I keep postponing it.

Copy link
Contributor Author

Please make different PRs. This one is quite big for review, I keep postponing it.

I have removed them.

Copy link
Member

iliakan commented Sep 10, 2020

Could you make the requested minor changes please?

Copy link
Contributor Author

Could you make the requested minor changes please?

Done.

Copy link
Member

iliakan commented Sep 10, 2020

I'll merge it and review the part about the task.

PLEASE make separate PRs for different articles =) instead of 1 huge PR (multiple branches help).

Copy link
Member

iliakan commented Sep 10, 2020

Thanks!

@iliakan iliakan merged commit 0168147 into javascript-tutorial:master Sep 10, 2020
Copy link
Member

iliakan commented Sep 10, 2020

@MuhammedZakir I thoroughly re-examined and further updated http://javascript.info/task/make-army, basing on your suggested changes, hope it's even better now.

Copy link
Contributor Author

MuhammedZakir commented Sep 10, 2020 •
edited
Loading

PLEASE make separate PRs for different articles =) instead of 1 huge PR (multiple branches help).

Sorry about that! It won't happen again! :-)

@MuhammedZakir I thoroughly re-examined and further updated http://javascript.info/task/make-army, basing on your suggested changes, hope it's even better now.

Looks good! 👍 I found some grammar mistakes. As I am a bit busy now, I will make a PR later.

Thanks!

You are welcome! :-)

Edit: Website is not updated. Are you perhaps updating it manually and not automatically on each new commit?

MuhammedZakir added a commit to MuhammedZakir/en.javascript.info that referenced this pull request Sep 18, 2020
MuhammedZakir added a commit to MuhammedZakir/en.javascript.info that referenced this pull request Sep 18, 2020 
Fix grammar and indent some paragraphs.
Complements javascript-tutorial#2081.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iliakan iliakan iliakan requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

AltStyle によって変換されたページ (->オリジナル) /